fix broken build cache caused by rustdoc builds #126934

onur-ozkan · 2024-06-25T08:44:45Z

Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g., x test miri && x test rustdoc && x test miri).

This change fixes that by moving on-broken-pipe into prepare_cargo_tool so it is set for all tools.

cc @RalfJung

Fixes #123177

rustbot · 2024-06-25T08:44:54Z

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

onur-ozkan · 2024-06-25T08:46:50Z

@clubby789 is not active these days and this is a contributor roadblock.

So, r? @Kobzol

RalfJung · 2024-06-25T09:03:04Z

The flag is also set here, is that expected duplication?

rust/src/bootstrap/src/core/build_steps/compile.rs

Lines 1009 to 1011 in 521e707

    
           // If the rustc output is piped to e.g. `head -n1` we want the process to be 
        
           // killed, rather than having an error bubble up and cause a panic. 
        
           cargo.rustflag("-Zon-broken-pipe=kill");

RalfJung · 2024-06-25T09:04:54Z

src/bootstrap/src/core/build_steps/tool.rs

-        // If the rustdoc output is piped to e.g. `head -n1` we want the process
-        // to be killed, rather than having an error bubble up and cause a
-        // panic.
-        cargo.rustflag("-Zon-broken-pipe=kill");


Looks like this was added in cde0cde, regressing the fix I made in #123192. Seems likely this will regress again.

Can you add a comment here saying to not add any rustflags, as that will cause undesired rebuilds?

Can you add a comment here saying to not add any rustflags, as that will cause undesired rebuilds?

We need to duplicate that comment for every impl Step for $tool to make it clear, which wouldn't be not good enough either (because people can miss comments sometimes). As I mentioned in #123177 (comment), I will create a follow-up PR later this week to have this automatically checked. So the test can ensure tools never break the build cache again.

🤷 IMO one can't have too many comments (or at least, that is very hard). Tests are nice but they only catch things after someone already made their PR, when CI runs. And tests can't cover all combinations of tools either.

We definitely need a change that ensures we never end up breaking the cache again (for any tool not just rustdoc).

Added a comment for rustdoc as it's harmless to have.

onur-ozkan · 2024-06-25T09:14:35Z

The flag is also set here, is that expected duplication?

rust/src/bootstrap/src/core/build_steps/compile.rs

Lines 1009 to 1011 in 521e707

// If the rustc output is piped to e.g. `head -n1` we want the process to be

// killed, rather than having an error bubble up and cause a panic.

cargo.rustflag("-Zon-broken-pipe=kill");

This PR changes rustflags for tools and the one you linked is for rustc builds. So, it is expected.

Kobzol · 2024-06-25T09:31:31Z

I wonder if we could return some "sealed" variant of CargoCommand from prepare_tool_cargo, so that it is not possible to modify its rustflags after creation anymore, to prevent issues like this. But it's probably only a problem for tools that are actually a part of a sysroot, like rustdoc, lld-wrapper etc. Maybe we could somehow mark these (in the type system) to make sure that it is not possible to break the cache.

But in any case, that's a more of a long-term goal, this PR looks reasonable as a hotfix.

Kobzol · 2024-06-25T09:44:08Z

The comment is a (slight) improvement over the status quo. Feel free to r=me.

onur-ozkan · 2024-06-25T09:56:48Z

@bors r=Kobzol rollup

bors · 2024-06-25T09:56:51Z

📌 Commit 2e017f4 has been approved by Kobzol

It is now in the queue for this repository.

…obzol fix broken build cache caused by rustdoc builds Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g., `x test miri && x test rustdoc && x test miri`). This change fixes that by moving `on-broken-pipe` into `prepare_cargo_tool` so it is set for all tools. cc `@RalfJung` Fixes rust-lang#123177

…mpiler-errors Rollup of 7 pull requests Successful merges: - rust-lang#126893 (Eliminate the distinction between PREC_POSTFIX and PREC_PAREN precedence level) - rust-lang#126907 (Fixes for 32-bit SPARC on Linux) - rust-lang#126932 (Tweak `FlatPat::new` to avoid a temporarily-invalid state) - rust-lang#126934 (fix broken build cache caused by rustdoc builds) - rust-lang#126943 (De-duplicate all consecutive native libs regardless of their options) - rust-lang#126946 (Add missing slash in `const_eval_select` doc comment) - rust-lang#126947 (Delegation: ast lowering refactor) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-06-25T16:02:08Z

failed in #126950
@bors r-

Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g., `x test miri && x test rustdoc && x test miri`). This change fixes that by moving `on-broken-pipe` into `prepare_cargo_tool` so it is set for all tools. Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2024-06-26T05:28:08Z

@bors r+ rollup=iffy p=1

bors · 2024-06-26T05:28:10Z

📌 Commit b3fb67e has been approved by onur-ozkan

It is now in the queue for this repository.

onur-ozkan · 2024-06-26T05:28:19Z

@bors r-

onur-ozkan · 2024-06-26T05:28:27Z

@bors r=Kobzol

bors · 2024-06-26T05:28:29Z

📌 Commit b3fb67e has been approved by Kobzol

It is now in the queue for this repository.

bors · 2024-06-26T09:42:36Z

⌛ Testing commit b3fb67e with merge bae813a...

bors · 2024-06-26T12:04:56Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing bae813a to master...

rust-timer · 2024-06-26T13:34:41Z

Finished benchmarking commit (bae813a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[3.1%, 3.1%]	1

Cycles

Results (primary 1.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.6%, 1.6%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 694.92s -> 695.46s (0.08%)
Artifact size: 326.70 MiB -> 326.69 MiB (-0.00%)

rustbot assigned clubby789 Jun 25, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 25, 2024

rustbot assigned Kobzol and unassigned clubby789 Jun 25, 2024

onur-ozkan mentioned this pull request Jun 25, 2024

Running ./x.py test miri --stage 0 twice rebuilds miri, cargo-miri, and rustdoc #123177

Closed

RalfJung reviewed Jun 25, 2024

View reviewed changes

onur-ozkan force-pushed the broken-build-cache branch 2 times, most recently from 191f594 to 2e017f4 Compare June 25, 2024 09:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2024

compiler-errors mentioned this pull request Jun 25, 2024

Rollup of 7 pull requests #126950

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 25, 2024

onur-ozkan force-pushed the broken-build-cache branch 2 times, most recently from 31e997a to 808ac1a Compare June 26, 2024 04:56

This comment has been minimized.

Sign in to view

onur-ozkan force-pushed the broken-build-cache branch from 808ac1a to b3fb67e Compare June 26, 2024 05:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 26, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 26, 2024

bors merged commit bae813a into rust-lang:master Jun 26, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jun 26, 2024

onur-ozkan deleted the broken-build-cache branch June 26, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix broken build cache caused by rustdoc builds #126934

fix broken build cache caused by rustdoc builds #126934

onur-ozkan commented Jun 25, 2024 •

edited

Loading

rustbot commented Jun 25, 2024

onur-ozkan commented Jun 25, 2024

RalfJung commented Jun 25, 2024

RalfJung Jun 25, 2024

onur-ozkan Jun 25, 2024 •

edited

Loading

RalfJung Jun 25, 2024

onur-ozkan Jun 25, 2024

onur-ozkan commented Jun 25, 2024

Kobzol commented Jun 25, 2024

Kobzol commented Jun 25, 2024

onur-ozkan commented Jun 25, 2024

bors commented Jun 25, 2024

matthiaskrgr commented Jun 25, 2024

This comment has been minimized.

onur-ozkan commented Jun 26, 2024

bors commented Jun 26, 2024

onur-ozkan commented Jun 26, 2024

onur-ozkan commented Jun 26, 2024

bors commented Jun 26, 2024

bors commented Jun 26, 2024

bors commented Jun 26, 2024

rust-timer commented Jun 26, 2024

fix broken build cache caused by rustdoc builds #126934

fix broken build cache caused by rustdoc builds #126934

Conversation

onur-ozkan commented Jun 25, 2024 • edited Loading

rustbot commented Jun 25, 2024

onur-ozkan commented Jun 25, 2024

RalfJung commented Jun 25, 2024

RalfJung Jun 25, 2024

Choose a reason for hiding this comment

onur-ozkan Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Jun 25, 2024

Choose a reason for hiding this comment

onur-ozkan Jun 25, 2024

Choose a reason for hiding this comment

onur-ozkan commented Jun 25, 2024

Kobzol commented Jun 25, 2024

Kobzol commented Jun 25, 2024

onur-ozkan commented Jun 25, 2024

bors commented Jun 25, 2024

matthiaskrgr commented Jun 25, 2024

This comment has been minimized.

onur-ozkan commented Jun 26, 2024

bors commented Jun 26, 2024

onur-ozkan commented Jun 26, 2024

onur-ozkan commented Jun 26, 2024

bors commented Jun 26, 2024

bors commented Jun 26, 2024

bors commented Jun 26, 2024

rust-timer commented Jun 26, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

onur-ozkan commented Jun 25, 2024 •

edited

Loading

onur-ozkan Jun 25, 2024 •

edited

Loading